Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1302 by updating the warning logs in WebClient to be consistent with Node SDK #1303

Merged
merged 2 commits into from
Nov 18, 2022

Conversation

mar3mar3
Copy link
Contributor

@mar3mar3 mar3mar3 commented Nov 18, 2022

Summary

This pull request resolves #1302

Category (place an x in each of the [ ])

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs-src (Documents, have you run ./scripts/docs.sh?)
  • /docs-src-v2 (Documents, have you run ./scripts/docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

slack_sdk/web/internal_utils.py Outdated Show resolved Hide resolved
attachments is not None
and isinstance(attachments, list)
and not all(
if attachments is not None and isinstance(attachments, list):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you applied black code formatter to this change? If not, can you run pip install black && black slack_sdk/?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that.
By the way, I'm editing in IntelliJ.

$pip install black && black slack_sdk/
Requirement already satisfied: black in ./venv/lib/python3.11/site-packages (22.8.0)
Requirement already satisfied: click>=8.0.0 in ./venv/lib/python3.11/site-packages (from black) (8.0.4)
Requirement already satisfied: platformdirs>=2 in ./venv/lib/python3.11/site-packages (from black) (2.5.4)
Requirement already satisfied: pathspec>=0.9.0 in ./venv/lib/python3.11/site-packages (from black) (0.10.2)
Requirement already satisfied: mypy-extensions>=0.4.3 in ./venv/lib/python3.11/site-packages (from black) (0.4.3)
All done! ✨ 🍰 ✨
113 files left unchanged.

Copy link
Member

@seratch seratch Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

By the way, I'm editing in IntelliJ

I like this. I am a long-time user of PyCharm for Python too!

@seratch seratch self-assigned this Nov 18, 2022
@seratch seratch added this to the 3.19.5 milestone Nov 18, 2022
@seratch seratch added enhancement M-T: A feature request for new functionality web-client Version: 3x labels Nov 18, 2022
@seratch seratch changed the title The implementation of #1208 is different from the node-slack-sdk vers… Fix #1302 by updating the warning logs in WebClient to be consistent with Node SDK Nov 18, 2022
@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Merging #1303 (a063b08) into main (46ece5e) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1303      +/-   ##
==========================================
+ Coverage   85.73%   85.80%   +0.06%     
==========================================
  Files          84       84              
  Lines        7846     7846              
==========================================
+ Hits         6727     6732       +5     
+ Misses       1119     1114       -5     
Impacted Files Coverage Δ
slack_sdk/socket_mode/builtin/internals.py 72.53% <0.00%> (+2.14%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Indeed it is, thank you!

Co-authored-by: Kazuhiro Sera <[email protected]>
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this contribution! Once all the CI jobs pass, we can merge this PR

@seratch seratch merged commit 0d23857 into slackapi:main Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality Version: 3x web-client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The implementation of #1208 is different from the node-slack-sdk version
2 participants